Skip to content

feat: edit stored drafts in place across CLI, TUI, and web#110

Merged
bhekanik merged 5 commits into
mainfrom
feat/edit-draft-integration
Jul 11, 2026
Merged

feat: edit stored drafts in place across CLI, TUI, and web#110
bhekanik merged 5 commits into
mainfrom
feat/edit-draft-integration

Conversation

@bhekanik

@bhekanik bhekanik commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

What

Adds edit-draft-in-place: open an existing stored draft, edit it, and save it back under the same DraftId — no duplicate draft row, nothing discarded. Wired across all three clients on one daemon surface.

Layers

  • Foundation: Request::GetDraft/Request::UpdateDraft IPC + ResponseData::Draft; Store::update_draft (plain UPDATE that preserves created_at, only touches rows still in 'draft' status, never arms the FK cascade); draft_codec (render a Draft to a compose file and apply edits back).
  • CLI: mxr drafts edit <id> — fetches the draft, opens $EDITOR, validates, saves via UpdateDraft. Temp file goes to the private 0700/0600 scratch dir.
  • TUI: stored-drafts view with in-place edit; prepare_draft_edit/handle_draft_edit_status preserve the temp file and surface its path on any parse/update failure (no silent content loss).
  • Web: save_draft_local is now upsert-by-id (UpdateDraft first, SaveDraft fallback on not-found); the compose hook reuses intent.draftId instead of minting a new UUID when editing.

Error taxonomy

update_draft distinguishes not-found / mid-send / already-sent, so the daemon returns a precise reason and clients never silently no-op.

Integration notes

This branch is the 4 feature branches (foundation + CLI + TUI + web) rebased cleanly onto v0.6.5 (0 conflicts). Verified locally: cargo build -p mxr ok; tests pass for mxr-compose (46), mxr-store (190), mxr-protocol, mxr-tui (22), mxr-web (6), and the mxr daemon/CLI suite (incl. new drafts edit cli-help snapshot); clippy clean on compose/daemon/tui/web.

Guardrails honored

New capability is daemon IPC + CLI, with TUI/web on the same surface; compose uses $EDITOR; unsent draft content is written only to the private scratch dir.


Summary by cubic

Edit stored drafts in place across CLI, TUI, and web—updates the same DraftId instead of creating duplicates. Adds GetDraft/UpdateDraft IPC, store update-in-place, a shared compose round-trip codec, and saves temp files to a private 0700/0600 scratch dir.

  • New Features
    • CLI: mxr drafts edit <id> opens the draft in $EDITOR and saves via UpdateDraft (same DraftId); enforces account match and writes temp files to the private scratch dir.
    • TUI: stored-drafts modal (gE or command palette) lists local drafts; Enter/e edits in place and preserves the temp file path on parse/update failure.
    • Web: save_draft_local is now upsert-by-id (try UpdateDraft, fallback to SaveDraft); compose reuses intent.draftId when editing to avoid duplication.
    • Daemon/Store: new Request::GetDraft and Request::UpdateDraft; Store::update_draft preserves created_at, updates only 'draft' rows, and returns precise errors (not-found / sending / sent).
    • Compose: new draft_codec to render a Draft to compose-file text and apply edits back, shared by CLI/TUI/web.
    • Safety: temp compose files live in a private 0700/0600 scratch dir to avoid leaking unsent content.

Written for commit bbef87e. Summary will update on new commits.

Review in cubic

@vercel

vercel Bot commented Jul 11, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
mxr-mail Ready Ready Preview, Comment Jul 11, 2026 2:29am

@coderabbitai

coderabbitai Bot commented Jul 11, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@bhekanik, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 3 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c089df7f-ddbc-49cc-b1f5-a9ca9d1457f4

📥 Commits

Reviewing files that changed from the base of the PR and between ca2b24b and bbef87e.

⛔ Files ignored due to path filters (2)
  • crates/daemon/tests/snapshots/cli_help__cli_help_drafts.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/cli_help__cli_help_drafts_edit.snap is excluded by !**/*.snap
📒 Files selected for processing (32)
  • apps/web/src/features/compose/useComposeSession.ts
  • crates/compose/src/draft_codec.rs
  • crates/compose/src/lib.rs
  • crates/daemon/src/activity/mapper.rs
  • crates/daemon/src/cli/mod.rs
  • crates/daemon/src/commands/mutations/compose.rs
  • crates/daemon/src/commands/mutations/mod.rs
  • crates/daemon/src/handler/mod.rs
  • crates/daemon/src/handler/mutations.rs
  • crates/daemon/src/lib.rs
  • crates/daemon/tests/cli_help.rs
  • crates/protocol/src/lib.rs
  • crates/protocol/src/types.rs
  • crates/store/src/draft.rs
  • crates/store/src/tests.rs
  • crates/tui/src/action.rs
  • crates/tui/src/app/actions.rs
  • crates/tui/src/app/draw.rs
  • crates/tui/src/app/input.rs
  • crates/tui/src/app/mailbox_actions.rs
  • crates/tui/src/app/mod.rs
  • crates/tui/src/app/state/mod.rs
  • crates/tui/src/app/state/modals.rs
  • crates/tui/src/async_result.rs
  • crates/tui/src/compose_flow.rs
  • crates/tui/src/keybindings.rs
  • crates/tui/src/runner.rs
  • crates/tui/src/runner/tests/accounts_and_delivery.rs
  • crates/tui/src/ui/command_palette.rs
  • crates/tui/src/ui/drafts_modal.rs
  • crates/tui/src/ui/mod.rs
  • crates/web/src/routes_v6.rs
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/edit-draft-integration

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

13 issues found across 34 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="crates/daemon/src/handler/mod.rs">

<violation number="1" location="crates/daemon/src/handler/mod.rs:2215">
P0: `UpdateDraft` derives its authorization scope from the caller-supplied `draft.account_id` rather than the stored draft's actual account. Because `request_account_scope` returns early via `request_account_id` and never reaches the `draft_account_scope` branch for `UpdateDraft`, an agent restricted to account A can submit a draft ID belonging to account B while setting `draft.account_id = A` in the payload. The profile check passes for A, and `Store::update_draft` explicitly SETs `account_id` on the matched row, allowing the draft to be overwritten and reassigned across accounts.

Scope `UpdateDraft` from the stored draft ID instead: look up the existing draft's account in `request_account_scope` (or validate the payload's `account_id` against the stored row before updating), and consider omitting `account_id` from the UPDATE's SET list for in-place edits.</violation>
</file>

<file name="crates/compose/src/draft_codec.rs">

<violation number="1" location="crates/compose/src/draft_codec.rs:23">
P2: `format_addresses` emits display names verbatim into a comma-delimited address list without RFC 5322 quoting. For example, the name `"Doe, Jane"` renders as `Doe, Jane <jane@example.com>`, which contains an unquoted comma. When `parse_address_list` processes this on the round-trip, the comma is interpreted as an address separator, typically splitting the name into a malformed extra address or dropping part of the recipient. The `mail_parser`-based parser can handle quoted commas (proven by existing tests in `crates/mail-parse/src/lib.rs` and `crates/provider-gmail/src/parse.rs`), but `format_addresses` never applies quoted-string escaping, so the round-trip is broken for any contact name containing commas, angle brackets, or similar special characters.</violation>
</file>

<file name="crates/tui/src/app/mailbox_actions.rs">

<violation number="1" location="crates/tui/src/app/mailbox_actions.rs:697">
P2: The stored-drafts edit flow clones a draft straight from the modals (`ListDrafts` snapshot) into `pending_draft_edit_request` without fetching the current row via `Request::GetDraft`. After the user finishes in `$EDITOR`, the stale snapshot plus local edits are sent as `UpdateDraft`, and the store's `update_draft` is a plain UPDATE with no compare-and-swap. If another client has modified the draft in the meantime, those changes get silently overwritten. Consider issuing `GetDraft` for the selected `DraftId` right before calling `prepare_draft_edit`, so the editor opens the freshest state rather than a potentially stale `ListDrafts` cache.</violation>
</file>

<file name="crates/store/src/draft.rs">

<violation number="1" location="crates/store/src/draft.rs:160">
P1: `update_draft` performs an unconditional UPDATE with only `id` and `status` in the WHERE clause, so concurrent edits from two clients silently overwrite each other (lost-update). Consider adding an optimistic concurrency check on `updated_at` so the second updater receives a conflict instead of clobbering the first edit.</violation>
</file>

<file name="crates/web/src/routes_v6.rs">

<violation number="1" location="crates/web/src/routes_v6.rs:1966">
P2: The upsert fallback classifies daemon failures by an unstructured error-message substring (`"not found"`) rather than using a typed error variant or structured status code from the IPC layer. The PR description advertises a "precise UpdateDraft taxonomy" on the daemon side, but the web client discards that precision by grepping the display text of `BridgeError::Ipc`. This is fragile: a wording change in the daemon's error message will break the fallback, and any unrelated IPC error containing `"not found"` will incorrectly trigger a `SaveDraft` insert. Consider propagating a structured error code (e.g., an enum or numeric status) through the IPC/bridge layer so the client can match on a guaranteed contract instead of a free-text substring.</violation>

<violation number="2" location="crates/web/src/routes_v6.rs:1966">
P1: The `UpdateDraft`-to-`SaveDraft` fallback in `save_draft_local` can silently resurrect a draft that was deleted in another tab or client. Because the handler only receives a `Draft` payload with no creation intent or version, a \"not found\" error is ambiguous: it may mean the ID is new (intended insert) or that the row was deleted since this editor opened it (unintended resurrection). When multiple compose tabs reuse `intent.draftId` and one deletes the draft, a stale save from another tab will re-insert the deleted row via `SaveDraft`, making deletion non-durable.

**Recommendation**: Pass an explicit upsert intent or guard the insert path so it only runs for client-generated / explicitly-new drafts, not for IDs that originated from a `GetDraft` load.</violation>
</file>

<file name="crates/daemon/src/commands/mutations/compose.rs">

<violation number="1" location="crates/daemon/src/commands/mutations/compose.rs:671">
P1: Using a deterministic scratch filename per `DraftId` and unconditionally unlinking it before writing introduces a destructive race. If a prior edit failed, its recovery content (intended to be kept for the user) is deleted on retry. Worse, two concurrent `mxr drafts edit <id>` processes cause the second invocation to remove the first editor's file from under it, after which both editors may operate on conflicting inodes or the same replacement path. The scratch file should use a unique per-invocation name (e.g. with a random suffix) and cleanup should be scoped to that specific invocation, preserving any unrelated recovery file.</violation>

<violation number="2" location="crates/daemon/src/commands/mutations/compose.rs:680">
P2: Stored-draft editing bypasses the snippet expansion that the standard compose-save flow performs. In `finalize_compose`, `expand_compose_snippets_with_context` is called before saving, but `drafts_edit` passes the raw edited text straight into `apply_edited_compose_file`. Because `send_stored_draft` also does not expand snippets before sending, any snippet tokens added during an edit can be sent literally to recipients instead of being resolved. The edit path should expand snippets after the editor returns and before constructing the updated `Draft`, matching the fresh-compose behavior.</violation>

<violation number="3" location="crates/daemon/src/commands/mutations/compose.rs:685">
P2: The CLI edit path parses the compose file twice: once explicitly for validation and again inside `draft_codec::apply_edited_compose_file`. This is redundant and means parser behavior changes must be consistent across both paths. Consider refactoring `apply_edited_compose_file` to accept the already-parsed `(&ComposeFrontmatter, &str)` so the caller can reuse the parse it already performed.</violation>
</file>

<file name="crates/tui/src/compose_flow.rs">

<violation number="1" location="crates/tui/src/compose_flow.rs:243">
P2: Using `resolve_compose_account` for draft editing applies a send-capability eligibility check that is unnecessary for updating an existing draft. Drafts from disabled or non-sending accounts are listed by the daemon but cannot be edited in the TUI because this call rejects them. Use an account lookup that does not require `compose_account_eligible` for the edit path, or filter/label such drafts consistently in the modal.</violation>

<violation number="2" location="crates/tui/src/compose_flow.rs:253">
P1: Using a deterministic scratch filename based only on `DraftId` and unconditionally removing it before writing destroys previously preserved recovery files from failed edits and creates a race between concurrent TUI sessions editing the same draft. Consider a unique per-edit filename and retaining failed files instead.</violation>

<violation number="3" location="crates/tui/src/compose_flow.rs:324">
P2: On non-zero editor exit, the flow displays 'Edit cancelled' and queues deletion of the temp file, assuming the user intentionally discarded changes. But editors (or wrappers like `git rebase --exec` pipelines) can write to disk before exiting non-zero, so this path can silently discard actual edits. Also, when the editor fails to launch (`Err(error)`), the error message does not include the temp file path, unlike the parse/update failure paths which explicitly say 'Your edits are still at …'. Since the PR's design intent for this function is to preserve the temp file and surface its path (as evidenced by the careful error handling around parsing and `UpdateDraft` failures), these two branches are inconsistent and leave gaps for silent content loss.</violation>

<violation number="4" location="crates/tui/src/compose_flow.rs:327">
P2: When `UpdateDraft` returns an unexpected successful response shape, the scratch file is preserved but its path is not disclosed in the error message, making recovery impossible for the user. Include `data.path.display()` in this branch, matching the adjacent `Response::Error` and transport-error branches.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Request::GetSyncStatus { account_id } => Some(account_id),
Request::SendDraft { draft, .. }
| Request::SaveDraft { draft }
| Request::UpdateDraft { draft }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P0: UpdateDraft derives its authorization scope from the caller-supplied draft.account_id rather than the stored draft's actual account. Because request_account_scope returns early via request_account_id and never reaches the draft_account_scope branch for UpdateDraft, an agent restricted to account A can submit a draft ID belonging to account B while setting draft.account_id = A in the payload. The profile check passes for A, and Store::update_draft explicitly SETs account_id on the matched row, allowing the draft to be overwritten and reassigned across accounts.

Scope UpdateDraft from the stored draft ID instead: look up the existing draft's account in request_account_scope (or validate the payload's account_id against the stored row before updating), and consider omitting account_id from the UPDATE's SET list for in-place edits.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/daemon/src/handler/mod.rs, line 2215:

<comment>`UpdateDraft` derives its authorization scope from the caller-supplied `draft.account_id` rather than the stored draft's actual account. Because `request_account_scope` returns early via `request_account_id` and never reaches the `draft_account_scope` branch for `UpdateDraft`, an agent restricted to account A can submit a draft ID belonging to account B while setting `draft.account_id = A` in the payload. The profile check passes for A, and `Store::update_draft` explicitly SETs `account_id` on the matched row, allowing the draft to be overwritten and reassigned across accounts.

Scope `UpdateDraft` from the stored draft ID instead: look up the existing draft's account in `request_account_scope` (or validate the payload's `account_id` against the stored row before updating), and consider omitting `account_id` from the UPDATE's SET list for in-place edits.</comment>

<file context>
@@ -2204,6 +2212,7 @@ fn request_account_id(req: &Request) -> Option<&mxr_core::AccountId> {
         Request::GetSyncStatus { account_id } => Some(account_id),
         Request::SendDraft { draft, .. }
         | Request::SaveDraft { draft }
+        | Request::UpdateDraft { draft }
         | Request::SaveDraftToServer { draft }
         | Request::CheckDraftSafety { draft, .. }
</file context>

Comment thread crates/store/src/draft.rs
// `created_at` is intentionally absent from SET so the original
// creation time survives the edit. Plain UPDATE (not INSERT OR
// REPLACE) so the account_id FK's ON DELETE CASCADE is never armed.
let result = sqlx::query(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: update_draft performs an unconditional UPDATE with only id and status in the WHERE clause, so concurrent edits from two clients silently overwrite each other (lost-update). Consider adding an optimistic concurrency check on updated_at so the second updater receives a conflict instead of clobbering the first edit.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/store/src/draft.rs, line 160:

<comment>`update_draft` performs an unconditional UPDATE with only `id` and `status` in the WHERE clause, so concurrent edits from two clients silently overwrite each other (lost-update). Consider adding an optimistic concurrency check on `updated_at` so the second updater receives a conflict instead of clobbering the first edit.</comment>

<file context>
@@ -133,6 +133,56 @@ impl super::Store {
+        // `created_at` is intentionally absent from SET so the original
+        // creation time survives the edit. Plain UPDATE (not INSERT OR
+        // REPLACE) so the account_id FK's ON DELETE CASCADE is never armed.
+        let result = sqlx::query(
+            "UPDATE drafts
+                SET account_id = ?, in_reply_to = ?, intent = ?,
</file context>

.await
{
Ok(response) => passthrough(response),
Err(BridgeError::Ipc(message)) if message.to_lowercase().contains("not found") => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: The UpdateDraft-to-SaveDraft fallback in save_draft_local can silently resurrect a draft that was deleted in another tab or client. Because the handler only receives a Draft payload with no creation intent or version, a "not found" error is ambiguous: it may mean the ID is new (intended insert) or that the row was deleted since this editor opened it (unintended resurrection). When multiple compose tabs reuse intent.draftId and one deletes the draft, a stale save from another tab will re-insert the deleted row via SaveDraft, making deletion non-durable.

Recommendation: Pass an explicit upsert intent or guard the insert path so it only runs for client-generated / explicitly-new drafts, not for IDs that originated from a GetDraft load.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/web/src/routes_v6.rs, line 1966:

<comment>The `UpdateDraft`-to-`SaveDraft` fallback in `save_draft_local` can silently resurrect a draft that was deleted in another tab or client. Because the handler only receives a `Draft` payload with no creation intent or version, a \"not found\" error is ambiguous: it may mean the ID is new (intended insert) or that the row was deleted since this editor opened it (unintended resurrection). When multiple compose tabs reuse `intent.draftId` and one deletes the draft, a stale save from another tab will re-insert the deleted row via `SaveDraft`, making deletion non-durable.

**Recommendation**: Pass an explicit upsert intent or guard the insert path so it only runs for client-generated / explicitly-new drafts, not for IDs that originated from a `GetDraft` load.</comment>

<file context>
@@ -1942,20 +1942,39 @@ async fn send_stored_draft(
+    .await
+    {
+        Ok(response) => passthrough(response),
+        Err(BridgeError::Ipc(message)) if message.to_lowercase().contains("not found") => {
+            let response = dispatch(
+                &state,
</file context>

let content = mxr_compose::draft_codec::draft_to_compose_file(&draft, &from)?;
// A draft is unsent mail content; write it to the per-user private 0700
// scratch dir with 0600 perms rather than the world-readable temp root.
let path = mxr_compose::private_tmp::private_scratch_dir()?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Using a deterministic scratch filename per DraftId and unconditionally unlinking it before writing introduces a destructive race. If a prior edit failed, its recovery content (intended to be kept for the user) is deleted on retry. Worse, two concurrent mxr drafts edit <id> processes cause the second invocation to remove the first editor's file from under it, after which both editors may operate on conflicting inodes or the same replacement path. The scratch file should use a unique per-invocation name (e.g. with a random suffix) and cleanup should be scoped to that specific invocation, preserving any unrelated recovery file.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/daemon/src/commands/mutations/compose.rs, line 671:

<comment>Using a deterministic scratch filename per `DraftId` and unconditionally unlinking it before writing introduces a destructive race. If a prior edit failed, its recovery content (intended to be kept for the user) is deleted on retry. Worse, two concurrent `mxr drafts edit <id>` processes cause the second invocation to remove the first editor's file from under it, after which both editors may operate on conflicting inodes or the same replacement path. The scratch file should use a unique per-invocation name (e.g. with a random suffix) and cleanup should be scoped to that specific invocation, preserving any unrelated recovery file.</comment>

<file context>
@@ -638,6 +638,86 @@ pub async fn drafts_discard(draft_id: String, account: Option<String>) -> anyhow
+    let content = mxr_compose::draft_codec::draft_to_compose_file(&draft, &from)?;
+    // A draft is unsent mail content; write it to the per-user private 0700
+    // scratch dir with 0600 perms rather than the world-readable temp root.
+    let path = mxr_compose::private_tmp::private_scratch_dir()?
+        .join(format!("mxr-draft-edit-{}.md", draft.id));
+    // Clear any leftover from a prior aborted edit so the O_EXCL write succeeds.
</file context>

// Best-effort: clear a stale file left behind by a previous
// cancelled/failed edit of this same draft — `write_private` uses
// O_EXCL and would otherwise refuse to write.
let _ = tokio::fs::remove_file(&path).await;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Using a deterministic scratch filename based only on DraftId and unconditionally removing it before writing destroys previously preserved recovery files from failed edits and creates a race between concurrent TUI sessions editing the same draft. Consider a unique per-edit filename and retaining failed files instead.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/tui/src/compose_flow.rs, line 253:

<comment>Using a deterministic scratch filename based only on `DraftId` and unconditionally removing it before writing destroys previously preserved recovery files from failed edits and creates a race between concurrent TUI sessions editing the same draft. Consider a unique per-edit filename and retaining failed files instead.</comment>

<file context>
@@ -229,6 +230,124 @@ pub(crate) async fn handle_compose_action(
+    // Best-effort: clear a stale file left behind by a previous
+    // cancelled/failed edit of this same draft — `write_private` uses
+    // O_EXCL and would otherwise refuse to write.
+    let _ = tokio::fs::remove_file(&path).await;
+    mxr_compose::private_tmp::write_private_async(&path, content.as_bytes())
+        .await
</file context>

validate_compose_draft(&frontmatter, &body, false)?;

let updated =
mxr_compose::draft_codec::apply_edited_compose_file(&draft, &edited, chrono::Utc::now())?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: The CLI edit path parses the compose file twice: once explicitly for validation and again inside draft_codec::apply_edited_compose_file. This is redundant and means parser behavior changes must be consistent across both paths. Consider refactoring apply_edited_compose_file to accept the already-parsed (&ComposeFrontmatter, &str) so the caller can reuse the parse it already performed.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/daemon/src/commands/mutations/compose.rs, line 685:

<comment>The CLI edit path parses the compose file twice: once explicitly for validation and again inside `draft_codec::apply_edited_compose_file`. This is redundant and means parser behavior changes must be consistent across both paths. Consider refactoring `apply_edited_compose_file` to accept the already-parsed `(&ComposeFrontmatter, &str)` so the caller can reuse the parse it already performed.</comment>

<file context>
@@ -638,6 +638,86 @@ pub async fn drafts_discard(draft_id: String, account: Option<String>) -> anyhow
+    validate_compose_draft(&frontmatter, &body, false)?;
+
+    let updated =
+        mxr_compose::draft_codec::apply_edited_compose_file(&draft, &edited, chrono::Utc::now())?;
+
+    expect_ack(
</file context>

let editor = mxr_compose::editor::resolve_editor(None);
mxr_compose::editor::spawn_editor(&editor, &path, None).await?;

let edited = std::fs::read_to_string(&path)?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Stored-draft editing bypasses the snippet expansion that the standard compose-save flow performs. In finalize_compose, expand_compose_snippets_with_context is called before saving, but drafts_edit passes the raw edited text straight into apply_edited_compose_file. Because send_stored_draft also does not expand snippets before sending, any snippet tokens added during an edit can be sent literally to recipients instead of being resolved. The edit path should expand snippets after the editor returns and before constructing the updated Draft, matching the fresh-compose behavior.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/daemon/src/commands/mutations/compose.rs, line 680:

<comment>Stored-draft editing bypasses the snippet expansion that the standard compose-save flow performs. In `finalize_compose`, `expand_compose_snippets_with_context` is called before saving, but `drafts_edit` passes the raw edited text straight into `apply_edited_compose_file`. Because `send_stored_draft` also does not expand snippets before sending, any snippet tokens added during an edit can be sent literally to recipients instead of being resolved. The edit path should expand snippets after the editor returns and before constructing the updated `Draft`, matching the fresh-compose behavior.</comment>

<file context>
@@ -638,6 +638,86 @@ pub async fn drafts_discard(draft_id: String, account: Option<String>) -> anyhow
+    let editor = mxr_compose::editor::resolve_editor(None);
+    mxr_compose::editor::spawn_editor(&editor, &path, None).await?;
+
+    let edited = std::fs::read_to_string(&path)?;
+    let (frontmatter, body) = mxr_compose::frontmatter::parse_compose_file(&edited)?;
+    validate_compose_draft(&frontmatter, &body, false)?;
</file context>

bg: &mpsc::UnboundedSender<IpcRequest>,
draft: Draft,
) -> Result<DraftEditReadyData, MxrError> {
let account = resolve_compose_account(bg, Some(&draft.account_id)).await?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Using resolve_compose_account for draft editing applies a send-capability eligibility check that is unnecessary for updating an existing draft. Drafts from disabled or non-sending accounts are listed by the daemon but cannot be edited in the TUI because this call rejects them. Use an account lookup that does not require compose_account_eligible for the edit path, or filter/label such drafts consistently in the modal.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/tui/src/compose_flow.rs, line 243:

<comment>Using `resolve_compose_account` for draft editing applies a send-capability eligibility check that is unnecessary for updating an existing draft. Drafts from disabled or non-sending accounts are listed by the daemon but cannot be edited in the TUI because this call rejects them. Use an account lookup that does not require `compose_account_eligible` for the edit path, or filter/label such drafts consistently in the modal.</comment>

<file context>
@@ -229,6 +230,124 @@ pub(crate) async fn handle_compose_action(
+    bg: &mpsc::UnboundedSender<IpcRequest>,
+    draft: Draft,
+) -> Result<DraftEditReadyData, MxrError> {
+    let account = resolve_compose_account(bg, Some(&draft.account_id)).await?;
+    let content = mxr_compose::draft_codec::draft_to_compose_file(&draft, &account.email)
+        .map_err(|e| MxrError::Ipc(e.to_string()))?;
</file context>

),
);
}
Ok(_) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: On non-zero editor exit, the flow displays 'Edit cancelled' and queues deletion of the temp file, assuming the user intentionally discarded changes. But editors (or wrappers like git rebase --exec pipelines) can write to disk before exiting non-zero, so this path can silently discard actual edits. Also, when the editor fails to launch (Err(error)), the error message does not include the temp file path, unlike the parse/update failure paths which explicitly say 'Your edits are still at …'. Since the PR's design intent for this function is to preserve the temp file and surface its path (as evidenced by the careful error handling around parsing and UpdateDraft failures), these two branches are inconsistent and leave gaps for silent content loss.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/tui/src/compose_flow.rs, line 324:

<comment>On non-zero editor exit, the flow displays 'Edit cancelled' and queues deletion of the temp file, assuming the user intentionally discarded changes. But editors (or wrappers like `git rebase --exec` pipelines) can write to disk before exiting non-zero, so this path can silently discard actual edits. Also, when the editor fails to launch (`Err(error)`), the error message does not include the temp file path, unlike the parse/update failure paths which explicitly say 'Your edits are still at …'. Since the PR's design intent for this function is to preserve the temp file and surface its path (as evidenced by the careful error handling around parsing and `UpdateDraft` failures), these two branches are inconsistent and leave gaps for silent content loss.</comment>

<file context>
@@ -229,6 +230,124 @@ pub(crate) async fn handle_compose_action(
+                        ),
+                    );
+                }
+                Ok(_) => {
+                    app.report_error(
+                        "Update Draft Failed",
</file context>

Ok(_) => {
app.report_error(
"Update Draft Failed",
"Unexpected daemon response to UpdateDraft".to_string(),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: When UpdateDraft returns an unexpected successful response shape, the scratch file is preserved but its path is not disclosed in the error message, making recovery impossible for the user. Include data.path.display() in this branch, matching the adjacent Response::Error and transport-error branches.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/tui/src/compose_flow.rs, line 327:

<comment>When `UpdateDraft` returns an unexpected successful response shape, the scratch file is preserved but its path is not disclosed in the error message, making recovery impossible for the user. Include `data.path.display()` in this branch, matching the adjacent `Response::Error` and transport-error branches.</comment>

<file context>
@@ -229,6 +230,124 @@ pub(crate) async fn handle_compose_action(
+                Ok(_) => {
+                    app.report_error(
+                        "Update Draft Failed",
+                        "Unexpected daemon response to UpdateDraft".to_string(),
+                    );
+                }
</file context>
Suggested change
"Unexpected daemon response to UpdateDraft".to_string(),
format!("Unexpected daemon response to UpdateDraft\n\nYour edits are still at {}", data.path.display()),

bhekanik added 5 commits July 11, 2026 03:26
save-local now tries UpdateDraft before falling back to SaveDraft, so
the endpoint upserts by id. The compose session reuses the restored
draft's id when scheduling a send on an edited draft, instead of
minting a new UUID that orphaned the original stored draft.
Opens the draft in $EDITOR via draft_to_compose_file/apply_edited_compose_file
and saves through Request::UpdateDraft, keeping the same DraftId (created_at
preserved, updated_at bumped). Uses the plain std::env::temp_dir() pattern
already used elsewhere in compose.rs for the scratch file, since the
private_tmp module referenced in the task foundation isn't present on this
branch yet.
Add a stored-drafts modal (gE / command palette "Drafts") that lists
locally-saved drafts across all accounts via Request::ListDrafts.
Pressing e/Enter on a selected draft resolves the sending account,
renders it to a compose file via mxr_compose::draft_codec, and opens
it in $EDITOR. On save, the edited content is parsed back and applied
onto the existing draft (preserving id/account_id/created_at) and
saved via Request::UpdateDraft, so editing a draft no longer creates
a duplicate.

Also brings in the mxr_compose::private_tmp module (private 0700
scratch dir, 0600 O_EXCL file writes) that the edit-draft foundation
depends on but that this branch didn't yet have, and fixes a clippy
lint (io_other_error) introduced by that addition.
@bhekanik bhekanik force-pushed the feat/edit-draft-integration branch from 89811fa to bbef87e Compare July 11, 2026 02:26
@bhekanik bhekanik merged commit 6101d95 into main Jul 11, 2026
27 of 28 checks passed
@bhekanik bhekanik deleted the feat/edit-draft-integration branch July 11, 2026 02:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant